Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use TranscriptRng for random number generation #109

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Jan 9, 2024

As noted in #108, it's possible to improve the robustness of prover nonces by using Merlin's TranscriptRng functionality.

This PR moves all prover nonce generation to this design. In particular, it ensures that the most up-to-date state of the transcript is used each time the prover needs secure randomness (per the Merlin documentation). The verifier also uses this to produce its random weights.

It also takes the opportunity to refactor transcript operations so they are cleaner and likely safer to use.

Closes #108.

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

One optimisation question: how do you interpret this note in the docs? Since the witness data cannot be updated within the prove_with_rng function (immutable reference), does the TranscriptRng need to be reinitialized multiple times within the function? It's not clear to me if they mean the TranscriptRng should be kept up to date with the witness data or if each time it is used once to generate a random number a new instance is needed (seems doubtful/doesn't make sense, but if that is the case this is not done everywhere e.g. lines 524,525 etc).

src/range_witness.rs Outdated Show resolved Hide resolved
@AaronFeickert
Copy link
Contributor Author

Since the witness data cannot be updated within the prove_with_rng function (immutable reference), does the TranscriptRng need to be reinitialized multiple times within the function?

Unfortunately yes. It's built from a clone of the current Transcript state, which is updated each round. This means reinitialization is necessary to take advantage of those state changes.

It's not clear to me if they mean the TranscriptRng should be kept up to date with the witness data or if each time it is used once to generate a random number a new instance is needed (seems doubtful/doesn't make sense, but if that is the case this is not done everywhere e.g. lines 524,525 etc).

The witness data is fixed at the start of proof generation and doesn't change. The only reason it needs to be added to each TranscriptRng reinitialization is because it isn't (and can't be) part of the transcript state since it's known only to the prover. It makes it a bit awkward to use, but I get why they designed the API that way.

The linked documentation is a bit unclear when it advises reinitializing the TranscriptRng every time the protocol needs randomness. Because the transcript state is only updated between rounds, there is no advantage to reinitializing within a round. That's the approach I took: the TranscriptRng is reinitialized whenever the transcript state is updated, and used for all randomness needed during that round.

That being said, I could have made the intent more clear by including better comments and placing the reinitializations more directly at the state changes. I'll make a small update to do so, and to include your other review suggestion.

@AaronFeickert
Copy link
Contributor Author

I made more changes than originally expected in order to make transcript and random number generator functionality more clear.

In particular, the previous transcript helper functions have been moved into RangeProofTranscript to abstract away the underlying Merlin transcript from the prover and verifier. I also moved RangeWitness::to_bytes out of RangeWitness and into RangeProofTranscript since the resulting byte representation is not canonical and should never directly be used outside of this specific use case.

The prover and verifier transcript operations were correspondingly simplified, and each TranscriptRng initialization now appears alongside a transcript state update.

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM - code is much clearer

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi looking good, just some small changes, please.

src/range_proof.rs Outdated Show resolved Hide resolved
src/range_proof.rs Outdated Show resolved Hide resolved
src/range_proof.rs Outdated Show resolved Hide resolved
src/transcripts.rs Outdated Show resolved Hide resolved
@AaronFeickert
Copy link
Contributor Author

AaronFeickert commented Jan 11, 2024

After discussing with @hansieodendaal, it was agreed that it would be best to move the transcript RNG updates into RangeProofTranscript to ensure they aren't forgotten.

The most recent commit does this. Transcript initialization returns a RangeProofTranscript and a TranscriptRng; calls to the challenge functions update the TranscriptRng via a mutable reference.

The verifier now also uses a TranscriptRng for its random weights. This allows it to use the same API as the prover, except without passing a witness as secret data. This is still safe, since the external RNG provides entropy.

@AaronFeickert AaronFeickert changed the title feat: use TranscriptRng for prover nonces feat: use TranscriptRng for random number generation Jan 11, 2024
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/transcripts.rs Show resolved Hide resolved
src/transcripts.rs Show resolved Hide resolved
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

ACK

@hansieodendaal hansieodendaal merged commit 6f1aab6 into tari-project:main Jan 12, 2024
8 checks passed
@AaronFeickert AaronFeickert deleted the transcript-rng branch January 12, 2024 17:08
hansieodendaal pushed a commit that referenced this pull request Jan 15, 2024
Recent work in #109 uses Merlin's `TranscriptRng` functionality to
improve the handling of random number generation, and also refactors
transcript operations for safer use through a `RangeProofTranscript`
wrapper.

A
[suggestion](#109 (comment))
by @sdbondi recommends moving the `TranscriptRng` into
`RangeProofTranscript` so the prover and verifier aren't responsible for
it. This PR adds such a change.

It also updates `RangeProofTranscript` to hold a mutable reference to
the external random number generator. This ensures the prover and
verifier can't accidentally use it instead of the `TranscriptRng`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use TranscriptRng for prover nonces
4 participants